Implement control-flow routing: on_false, on_success, on_failure (#333) - #443
Conversation
Stacked on #442, which the routing tests use to assert skipped steps through the typed result. `on_false` and `on_success` did not exist at all. `condition:` was already evaluated and already skipped a step, so what was missing was purely the routing: where to go once the answer is known. Routing jumps forward and marks the steps in between as skipped, reusing `_skip_tasks_between` -- the machinery `goto` already had -- rather than adding a second one beside it. Skipping is not failing: a pipeline that routed around a step still reports success, and PipelineResult separates skipped_steps from failed_steps accordingly. The awkward part is `on_failure`, which already meant a failure *policy* (fail / continue / skip / retry) and now also names a step to jump to. Both readings are supported and disambiguated by value: a reserved policy word keeps its policy meaning, anything else is a step id, and when it names a step the run continues there instead of aborting. That is only safe because the genuinely ambiguous case is refused rather than guessed. A pipeline with a step whose id *is* a policy word -- a step called `retry` -- fails to compile, because `on_failure: retry` could not be told apart from selecting the policy. core/routing.py holds that single definition; the compiler and the executor both consume it. Routing targets are validated at compile time: a jump to a step that does not exist, or a step routing to itself, is exit 2 naming the target. That check went into validation/dependency_validator.py, which is the validator actually on the compile path -- my first attempt put it in compiler/schema_validator.py, whose validate_dependencies() nothing calls, so the tests failed while the code looked right. Only the schema widening stayed there: `on_failure` was constrained to an enum of the four policies, which would have rejected every routing target outright. The runtime decision uses StepResult.success, not TaskStatus. A tool returning {"success": False} without raising leaves its task COMPLETED, so routing on status alone sent a failing step down the on_success path -- the same distinction #442 had to draw, reused here rather than restated a third time. The contract is updated: ADR 0001 gains a control-flow section, and docs/actions.md is generated with the routing table and the policy list so the published vocabulary cannot drift from FAILURE_POLICIES. Blocking suite 490 -> 506 passed, 13 skipped, 0 failed, 0 xfailed. Collection 3285 -> 3301. Examples validating unchanged at 3 of 111. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
CI 9/9 green. The legacy tally moved, and the two tests responsible are flaky, not regressions — with evidence, since the aggregate alone could not tell the difference.
Neither is anywhere near control flow. Their history across recent runs:
Both pass locally on this branch when run directly. They flip across PRs that touched nothing related to them — including #441, which predates every line of this change. This corrects something I claimed on #440I said there that two consecutive The practical consequence for future comparisons: an aggregate delta of ±2 in this suite is not by itself evidence of anything. The named Worth filing the two as flaky separately; they are pre-existing and unrelated to routing. |
Closes #333. Stacked on #442 — the routing tests assert skipped steps through the typed result, so merge that first.
What was actually missing
condition:already evaluated and already skipped a step.on_falseandon_successdid not exist anywhere insrc/. So the gap was purely routing: where execution goes once the answer is known.Routing jumps forward and marks the steps in between as skipped, reusing
_skip_tasks_between— the machinerygotoalready had — rather than adding a second one beside it.on_falsecondition:was false, so it was skippedon_successon_failureSkipping is not failing: a pipeline that routed around a step still reports
success, andPipelineResultseparatesskipped_stepsfromfailed_stepsaccordingly.The
on_failurecollisionon_failurealready meant a failure policy —fail/continue/skip/retry— and #333 asks for the same key to name a step. Both readings are useful, so both are supported, disambiguated by value: a reserved policy word keeps its policy meaning, anything else is a step id, and when it names a step the run continues there instead of aborting.That is only safe because the genuinely ambiguous case is refused rather than guessed. A pipeline containing a step called
retryfails to compile —on_failure: retrycould not be told apart from selecting the policy.core/routing.pyholds the single definition; the compiler and the executor both consume it.Two things I got wrong first, both instructive
I put the compile-time validation in the wrong validator.
compiler/schema_validator.pyhas avalidate_dependencies()that nothing calls — the compile path usesvalidation/dependency_validator.py. My tests failed while the code read correctly. The check now lives in the live path only; the duplicate was reverted rather than left as a second implementation. What did have to stay inschema_validator.pyis the schema widening:on_failurewas constrained toenum: [continue, fail, retry, skip], which would have rejected every routing target outright.Routing on
TaskStatussent a failing step down the success path. A tool returning{"success": False}without raising leaves its taskCOMPLETED, sostatus is FAILEDnever matched andon_failurerouting silently did nothing — theneverstep ran and wrote its file. The runtime now asksStepResult.success, reusing the distinction #442 had to draw rather than restating it a third time.That is the same defect class for the third time in this sequence (#442's exit code, #441's error recording, here). It is worth stating plainly:
statusanswers "did it finish",successanswers "did it work", and anything that branches on the wrong one produces a silently wrong result rather than an error.Compile-time validation
Contract updated
ADR 0001 gains a control-flow section, and
docs/actions.mdis regenerated with the routing table and the policy list read fromFAILURE_POLICIES, so the published vocabulary cannot drift from the code. The docs drift test covers it.Numbers
uv lock --check/git diff --check/ docs drift16 new routing tests: each key taken and not taken, the skip representation, the side effect of a routed-over step not happening, policy-vs-step disambiguation across all four policy words, and all three compile-time refusals.
Not in this PR
#320 also bundles
experts:,selection_schema:andpersonality:— model-selection features that have nothing to do with control flow and would make this PR two unrelated changes. #335 (vars:structured outputs) is likewise separate. Both want their own PRs against the now-explicit contract.